Support ECParameters that contain only D (Win, Linux). #33874

vcsjones · 2020-03-20T16:48:10Z

These changes allows importing EC keys for ECDsa and ECDiffieHellman where the private key (D) is specified but the public key (Q) is not.

This change is to remove a limitation on ECPrivateKey which, by definition, is not required to carry the public key, only the private key. It is expected that such keys are able to re-derive the public key from the private key. Prior to this change, the pre-computed public key was required.

MacOS does not currently support this. CNG and OpenSSL do.

Contributes to #33744

If D (private key) is supplied but not the public key (Q), permit this and allow the platform to re-calculate the public key from the private key.

If Q is missing for an ECParameters structure, import it as PKCS8 and let CNG construct Q appropriately.

The *Cng types currently do not support this; only the implementation-detail types do.

src/libraries/Common/src/System/Security/Cryptography/EccKeyFormatHelper.cs

vcsjones · 2020-03-30T05:15:21Z

/cc @bartonjs

bartonjs · 2020-03-30T20:55:55Z

...stem.Security.Cryptography.Algorithms/src/System/Security/Cryptography/ECDiffieHellmanCng.cs

+
+                try
+                {
+                    if (!writer.TryEncode(pkcs8Buffer, out int encodedWritten))


Meta, for AsnWriter API purposes: I wonder if some sort of SendData<TState>(TState state, SpanAction<byte, TState> processor) would be useful to avoid this ceremony without over-exposing the Span.

Well, it turns out I didn't need any of this code anyway 😅.

~~I am curious why not make EncodeAsSpan public?~~

Okay yes I see how it might be a bad idea. I suppose a public method that returns a span and marks it no longer writable?

Oh, were you thinking something like this?

public void SendData<TState>(TState state, SpanAction<T, byte> processor) { // Validate processor(state, _buffer.AsSpan(0, _offset)); }

Yeah that would be useful, I would think.

bartonjs · 2020-03-31T15:37:59Z

I could not figure out a way to get CNG to accept a "no public key but has explicit parameters" key

Is that still true with the change to use Q=(0,0) blobs instead of PKCS#8?

vcsjones · 2020-03-31T16:11:04Z

@bartonjs

Is that still true with the change to use Q=(0,0) blobs instead of PKCS#8?

For ECParameters, "(0,0)" works for explicit curves. However for PKCS8, it does not work. CNG does it's own PKCS8 handling it appears. That is, when using NCRYPT_PKCS8_PRIVATE_KEY_BLOB, the import fails (with an access violation?)

Unhandled exception. Internal.Cryptography.CryptoThrowHelper+WindowsCryptographicException: Unknown error (0xc0000005)
   at System.Security.Cryptography.CngKey.Import(ReadOnlySpan`1 keyBlob, String curveName, CngKeyBlobFormat format, CngProvider provider)
   at System.Security.Cryptography.CngPkcs8.ImportPkcs8(ReadOnlySpan`1 keyBlob)
   at System.Security.Cryptography.CngPkcs8.ImportPkcs8PrivateKey(ReadOnlySpan`1 source, Int32& bytesRead)
   at System.Security.Cryptography.ECDsaCng.ImportPkcs8PrivateKey(ReadOnlySpan`1 source, Int32& bytesRead)
   at Test.Program.Main(String[] args) in D:\code\personal\scratch\Program.cs:line 20

using ECDsaCng e = new ECDsaCng();
string keyStr = @"
MIIBMwIBADCCAQMGByqGSM49AgEwgfcCAQEwLAYHKoZIzj0BAQIhAP////8AAAAB
AAAAAAAAAAAAAAAA////////////////MFsEIP////8AAAABAAAAAAAAAAAAAAAA
///////////////8BCBaxjXYqjqT57PrvVV2mIa8ZR0GsMxTsPY7zjw+J9JgSwMV
AMSdNgiG5wSTamZ44ROdJreBn36QBEEEaxfR8uEsQkf4vOblY6RA8ncDfYEt6zOg
9KE5RdiYwpZP40Li/hp/m47n60p8D54WK84zV2sxXs7LtkBoN79R9QIhAP////8A
AAAA//////////+85vqtpxeehPO5ysL8YyVRAgEBBCcwJQIBAQQgO/HNs4GTcN1s
MVZ2GRwFlvPnq2YEUrzTEcSyUEr6axY=";
byte[] key = Convert.FromBase64String(keyStr);
e.ImportPkcs8PrivateKey(key, out _);
// generated with:
// openssl ecparam -genkey -name prime256v1 -noout | openssl ec -param_enc explicit -no_public | openssl pkey

That leaves the following options.

Leave explicit parameters with no public keys unsupported.
Allow empty Q for explicit curves on ECParameters-style imports, but leave it not-working with PKCS8. This would fix ImportParameters but not the ECPrivateKey/PKCS8 paths, with is where we were hitting the issue in the first place.
Special-path "no public key" PKCS8 imports in CNG, decode it ourselves, import via ECParameters.

vcsjones · 2020-04-01T15:52:16Z

Re: MacOS, I can look at that in about a week when a new Mac Mini gets delivered. Until then, my MacBook just can't muscle through this.

bartonjs · 2020-04-01T16:09:31Z

For ECParameters, "(0,0)" works for explicit curves. However for PKCS8, it does not work

I've reached out to my contacts to see if we can get that state changed. In the meantime... so that we don't have to try supporting key state parity with their PKCS#8 attribute parsing maybe it's

Try PKCS#8 import, as-is. On success: done.
Crack the payload to see if it's explicit parameters with no public key (if not, let the error/exception go).
If the payload has no attributes: import the equivalent blob, done.
Import the equivalent blob, export it to get Q
Insert Q in the original PKCS#8, import that.

It's not pretty... but it works.

vcsjones · 2020-04-01T16:17:19Z

It's not pretty... but it works.

Makes sense. I think that will keep me busy for a bit. 😄

vcsjones · 2020-04-01T16:18:00Z

@bartonjs how much review have you given this? Is it okay for me to rebase / squash this branch?

bartonjs · 2020-04-01T16:25:46Z

how much review have you given this?

Fully, I think.

Is it okay for me to rebase / squash this branch?

If you need new stuff from master, merging would be preferred. (Sitting stale is fine)

If you expect that it's going to be effectively a total rewrite from here, then I guess things that require a force-push are OK.

vcsjones · 2020-04-01T16:29:00Z

If you expect that it's going to be effectively a total rewrite from here, then I guess things that require a force-push are OK.

No, I don't think that is the case. I'll stick with merge commits. I try not to let branches get too stale, at least hoping to be able to pick up some changes that fix test stability.

Now that it is a real feature, we want to test all other explicit scenarios.

vcsjones · 2020-04-01T18:18:48Z

@bartonjs This will be *NO REVIEW* for a while. I'll ping when ready.

vcsjones · 2020-04-04T19:40:16Z

@bartonjs I think this is ready for review again.

To clarify something: the CNG PKCS8 importer doesn't like no PublicKey on the ECPrivateKey just for explicit curves. However, I incorrectly determined that it needed the correctly computed public key. Pushing 0,0 into the PKCS8 for the public key is enough to get CNG happy again, so we can avoid round-tripping through ECParameters to get the right public key.

src/libraries/Common/src/System/Security/Cryptography/CngPkcs8.cs

bartonjs · 2020-04-05T19:12:00Z

Thanks for jumping on this, @vcsjones.

Dotnet-GitSync-Bot added the area-System.Security label Mar 20, 2020

vcsjones mentioned this pull request Mar 20, 2020

CryptographicException decoding TLS cert in 5.0.0 preview, linux only (Fine in 3.1.2) #33744

Closed

jaredpar mentioned this pull request Mar 21, 2020

Several tests fail on mono with mono_threads_pthread_kill: pthread_kill failed with error 11 #32377

Closed

vcsjones force-pushed the 33744-fix branch from f71efc9 to 0225fad Compare March 22, 2020 19:26

This was referenced Mar 23, 2020

Builds failing with "remote provider unable to process the request" #33965

Closed

WinHttpHandler test UseClientCertOnHttp2_OSSupportsIt_Success failing #34010

Closed

vcsjones force-pushed the 33744-fix branch 2 times, most recently from c61eea8 to 56ff6ac Compare March 27, 2020 20:04

vcsjones added 6 commits March 28, 2020 15:40

Support ECParameters that contain only D.

05613ae

If D (private key) is supplied but not the public key (Q), permit this and allow the platform to re-calculate the public key from the private key.

Support missing Q on Windows.

02581c7

If Q is missing for an ECParameters structure, import it as PKCS8 and let CNG construct Q appropriately.

Avoid complex logic.

7144a5e

Add PEM/PKCS8 encoded tests.

f387e9d

Fix tests for macOS

20c61ae

Fix build break.

99dbb01

vcsjones force-pushed the 33744-fix branch from 56ff6ac to 99dbb01 Compare March 28, 2020 19:49

vcsjones added 9 commits March 28, 2020 16:15

Only support named curves.

57a477f

Restore length check.

d60707a

Remove tests from the CNG types.

b892e17

The *Cng types currently do not support this; only the implementation-detail types do.

I think?

d89cd9d

Actually use the property.

e6987d2

Implement for CNG.

059e087

Encode to pooled buffer.

c8328f2

Fix syntax I will probably be told to change.

de3ff4a

Add tests for ImportParameters.

9a7f119

vcsjones changed the title ~~[WIP] Support ECParameters that contain only D.~~ Support ECParameters that contain only D. Mar 30, 2020

This comment has been minimized.

Sign in to view

vcsjones commented Mar 30, 2020

View reviewed changes

src/libraries/Common/src/System/Security/Cryptography/EccKeyFormatHelper.cs Show resolved Hide resolved

vcsjones marked this pull request as ready for review March 30, 2020 05:00

bartonjs reviewed Mar 30, 2020

View reviewed changes

vcsjones added 3 commits April 1, 2020 12:53

Support explicit curves lacking pre-computed public keys.

2700348

Merge remote-tracking branch 'ms/master' into 33744-fix

e485093

Bulk up on explicit curve tests.

e4ec783

Now that it is a real feature, we want to test all other explicit scenarios.

bartonjs added the NO-REVIEW Experimental/testing PR, do NOT review it label Apr 1, 2020

vcsjones mentioned this pull request Apr 2, 2020

API Proposal: ECPoint.IsEmpty #34459

Closed

This comment has been minimized.

Sign in to view

vcsjones added 6 commits April 4, 2020 10:51

Add ECPrivateKey to CNG.

b071145

Merge remote-tracking branch 'ms/master' into 33744-fix

497e233

CNG limited private explicit curve support on ECParameters.

7e71707

Use a zero value public key in PKCS8 where required

e47356b

Zero out D

629b43f

Add test for keyUsage.

457cb2b

bartonjs reviewed Apr 5, 2020

View reviewed changes

src/libraries/Common/src/System/Security/Cryptography/CngPkcs8.cs Show resolved Hide resolved

bartonjs removed the NO-REVIEW Experimental/testing PR, do NOT review it label Apr 5, 2020

bartonjs approved these changes Apr 5, 2020

View reviewed changes

bartonjs merged commit 6d395de into dotnet:master Apr 5, 2020

vcsjones deleted the 33744-fix branch April 5, 2020 19:12

vcsjones mentioned this pull request Apr 7, 2020

Support ECParameters that contain only D (macOS). #34654

Merged

ghost locked as resolved and limited conversation to collaborators Dec 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support ECParameters that contain only D (Win, Linux). #33874

Support ECParameters that contain only D (Win, Linux). #33874

vcsjones commented Mar 20, 2020 •

edited

Loading

This comment has been minimized.

vcsjones commented Mar 30, 2020

bartonjs Mar 30, 2020

vcsjones Mar 31, 2020 •

edited

Loading

vcsjones Apr 1, 2020

bartonjs commented Mar 31, 2020

vcsjones commented Mar 31, 2020

vcsjones commented Apr 1, 2020

bartonjs commented Apr 1, 2020

vcsjones commented Apr 1, 2020

vcsjones commented Apr 1, 2020 •

edited

Loading

bartonjs commented Apr 1, 2020

vcsjones commented Apr 1, 2020

vcsjones commented Apr 1, 2020

This comment has been minimized.

vcsjones commented Apr 4, 2020

bartonjs commented Apr 5, 2020

Support ECParameters that contain only D (Win, Linux). #33874

Support ECParameters that contain only D (Win, Linux). #33874

Conversation

vcsjones commented Mar 20, 2020 • edited Loading

This comment has been minimized.

vcsjones commented Mar 30, 2020

bartonjs Mar 30, 2020

Choose a reason for hiding this comment

vcsjones Mar 31, 2020 • edited Loading

Choose a reason for hiding this comment

vcsjones Apr 1, 2020

Choose a reason for hiding this comment

bartonjs commented Mar 31, 2020

vcsjones commented Mar 31, 2020

vcsjones commented Apr 1, 2020

bartonjs commented Apr 1, 2020

vcsjones commented Apr 1, 2020

vcsjones commented Apr 1, 2020 • edited Loading

bartonjs commented Apr 1, 2020

vcsjones commented Apr 1, 2020

vcsjones commented Apr 1, 2020

This comment has been minimized.

vcsjones commented Apr 4, 2020

bartonjs commented Apr 5, 2020

vcsjones commented Mar 20, 2020 •

edited

Loading

vcsjones Mar 31, 2020 •

edited

Loading

vcsjones commented Apr 1, 2020 •

edited

Loading